fix(lambda): use enum values for applicationLogLevel and systemLogLevel #29904

dali546 · 2024-04-19T10:23:17Z

Issue # (if applicable)

Reason for this change

Enumerate ApplicationLogLevel and SystemLogLevel to help with typing

Description of changes

Both fields should use the enum type for available options

Description of how you validated changes

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

dali546 · 2024-04-21T21:00:28Z

Exemption Request: minor typescript change

aaythapa · 2024-04-22T20:23:29Z

Hi @dali546, thanks for making this change! Not sure why it wasn't an enum before. I'll have to double check to see if this is backwards compatible. In the meantime looks like the build step is failing because the some integration test snapshots need updates. You can use yarn integ --update-on-failed to update the integ snapshots.

dali546 · 2024-04-22T20:56:22Z

Hi @aaythapa, thank you for taking a look at this PR... I've cloned down this repo, but unsure where to run yarn integ --update-on-failed? And it also suggests I should be authenticated with AWS for this too? Is this something I should be doing as a external contributor?

aaythapa · 2024-04-22T21:26:05Z

Hi @dali546, looking through the build log and I see API elements with incompatible change, this basically means that this PR in its current state is backwards incompatible :(.

You can enforce the enum type by making a new property (maybe called systemLogLevelV2) and have that property use the enum. You'd then deprecate the old systemLogLevel property. This would also fix the build failures you're seeing in the PR but you'd still have to add some integ tests.

This PR pretty much fixes the same issue you're trying to fix so feel free to use it as reference (note this PR doesn't deprecate the old property for some reason, this pr is a good example of adding a deprecated label). You can use our integ test guide to add new integ tests.

dali546 · 2024-04-24T22:31:08Z

Hi @aaythapa. I need a little help. Locally I am trying to run yarn run integ-runner --update-on-failed --directory packages/@aws-cdk-testing/test/aws-lambda but my results show as UNCHANGED for all of them. it's not generating a new snapshot

Any advice?

aaythapa · 2024-04-29T17:29:19Z

Hi @dali546, sorry about the delay. Not sure what the exact issue could be. You can try deleting the snapshot folder for the test and re-making it. I see that integ.logging-config.js test is failing, so you can delete that tests snapshot folder and re-run so it creates a new folder, hopefully the different snapshot will show up.

packages/aws-cdk-lib/aws-lambda/lib/function.ts

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

aaythapa

@dali546 looks good, just some small final comments.

packages/aws-cdk-lib/aws-lambda/test/logging-config.test.ts

dali546 · 2024-05-27T22:46:48Z

hi @dali546, this PR is really close, do you still have time to work on this? If not I can take it over the line

Hi @aaythapa , apologies. I will try and get it done this week, if not please takeover. I apologize for the delay

aaythapa · 2024-05-28T20:29:49Z

hi @dali546, this PR is really close, do you still have time to work on this? If not I can take it over the line

Hi @aaythapa , apologies. I will try and get it done this week, if not please takeover. I apologize for the delay

Np, this isn't urgent so I'll let you take this all the way to the finish line

aaythapa

thanks for adding this

mergify · 2024-05-29T22:56:19Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2024-05-29T23:24:41Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: a267400
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2024-05-29T23:25:09Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

…el (aws#29904) ### Issue # (if applicable) ### Reason for this change Enumerate `ApplicationLogLevel` and `SystemLogLevel` to help with typing ### Description of changes Both fields should use the enum type for available options ### Description of how you validated changes ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

aws-cdk-automation · 2024-07-25T21:14:37Z

Comments on closed issues and PRs are hard for our team to see. If you need help, please open a new issue that references this one.

github-actions bot added the p2 label Apr 19, 2024

aws-cdk-automation requested a review from a team April 19, 2024 10:23

github-actions bot added the beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK label Apr 19, 2024

aws-cdk-automation previously requested changes Apr 19, 2024

View reviewed changes

dali546 force-pushed the patch-1 branch from 7c096c9 to ac56646 Compare April 21, 2024 20:58

dali546 requested a review from aws-cdk-automation April 21, 2024 20:58

github-actions bot mentioned this pull request Apr 22, 2024

Weekly PR metrics report #29922

Closed

aaythapa self-assigned this Apr 22, 2024

dali546 force-pushed the patch-1 branch from ac56646 to 5286aba Compare April 22, 2024 20:56

dali546 force-pushed the patch-1 branch from 5286aba to eeb896c Compare April 24, 2024 21:48

dali546 force-pushed the patch-1 branch from eeb896c to fd01acb Compare April 24, 2024 22:40

This was referenced Apr 26, 2024

Weekly PR metrics report #29980

Closed

Weekly PR metrics report #29986

Closed

aaythapa reviewed Apr 29, 2024

View reviewed changes

packages/aws-cdk-lib/aws-lambda/lib/function.ts Show resolved Hide resolved

packages/aws-cdk-lib/aws-lambda/lib/function.ts Outdated Show resolved Hide resolved

dali546 force-pushed the patch-1 branch from fd01acb to 30f05bd Compare April 30, 2024 22:00

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Apr 30, 2024

github-actions bot mentioned this pull request May 1, 2024

Monthly PR metrics report #30026

Closed

dali546 requested a review from aaythapa May 4, 2024 21:14

dali546 force-pushed the patch-1 branch 3 times, most recently from f027aeb to bc42277 Compare May 6, 2024 14:12

aaythapa reviewed May 8, 2024

View reviewed changes

packages/aws-cdk-lib/aws-lambda/test/logging-config.test.ts Show resolved Hide resolved

packages/aws-cdk-lib/aws-lambda/test/logging-config.test.ts Outdated Show resolved Hide resolved

aaythapa changed the title ~~fix: use enum values for applicationLogLevel and systemLogLevel~~ fix(lambda): use enum values for applicationLogLevel and systemLogLevel May 8, 2024

dali546 added 5 commits May 28, 2024 22:42

fix: use enum values for applicationLogLevel and systemLogLevel

be0f883

chore: deprecate applicationLogLevel and systemLogLevel

ebcc1b8

fix: generate integ snapshot again

e6a69da

chore: refactor error handling

87a16fd

chore: add additional tests

f2d6e78

dali546 force-pushed the patch-1 branch from 3e8b49d to f2d6e78 Compare May 28, 2024 21:42

dali546 requested a review from aaythapa May 28, 2024 21:42

aaythapa approved these changes May 29, 2024

View reviewed changes

Merge branch 'main' into patch-1

a267400

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label May 29, 2024

mergify bot merged commit 3f53a45 into aws:main May 29, 2024
12 checks passed

dali546 deleted the patch-1 branch May 31, 2024 09:56

rwlxxvii mentioned this pull request Jun 22, 2024

[Snyk] Upgrade aws-cdk-lib from 2.139.0 to 2.144.0 rwlxxvii/containers#140

Merged

NOUIY mentioned this pull request Jun 22, 2024

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#105

Open

kazimanzurrashid mentioned this pull request Jun 29, 2024

[Snyk] Upgrade aws-cdk-lib from 2.144.0 to 2.145.0 kazimanzurrashid/aws-scheduler-go#821

Open

This was referenced Jun 29, 2024

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#106

Open

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#107

Open

This was referenced Jul 12, 2024

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#108

Open

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#109

Open

This was referenced Jul 19, 2024

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#110

Open

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#111

Open

aws locked as resolved and limited conversation to collaborators Jul 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(lambda): use enum values for applicationLogLevel and systemLogLevel #29904

fix(lambda): use enum values for applicationLogLevel and systemLogLevel #29904

dali546 commented Apr 19, 2024 •

edited

Loading

aws-cdk-automation left a comment

dali546 commented Apr 21, 2024 •

edited

Loading

aaythapa commented Apr 22, 2024

dali546 commented Apr 22, 2024

aaythapa commented Apr 22, 2024 •

edited

Loading

dali546 commented Apr 24, 2024

aaythapa commented Apr 29, 2024

aaythapa left a comment •

edited

Loading

dali546 commented May 27, 2024

aaythapa commented May 28, 2024

aaythapa left a comment

mergify bot commented May 29, 2024

aws-cdk-automation commented May 29, 2024

mergify bot commented May 29, 2024

aws-cdk-automation commented Jul 25, 2024

fix(lambda): use enum values for applicationLogLevel and systemLogLevel #29904

fix(lambda): use enum values for applicationLogLevel and systemLogLevel #29904

Conversation

dali546 commented Apr 19, 2024 • edited Loading

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Checklist

aws-cdk-automation left a comment

Choose a reason for hiding this comment

dali546 commented Apr 21, 2024 • edited Loading

aaythapa commented Apr 22, 2024

dali546 commented Apr 22, 2024

aaythapa commented Apr 22, 2024 • edited Loading

dali546 commented Apr 24, 2024

aaythapa commented Apr 29, 2024

aaythapa left a comment • edited Loading

Choose a reason for hiding this comment

dali546 commented May 27, 2024

aaythapa commented May 28, 2024

aaythapa left a comment

Choose a reason for hiding this comment

mergify bot commented May 29, 2024

aws-cdk-automation commented May 29, 2024

AWS CodeBuild CI Report

mergify bot commented May 29, 2024

aws-cdk-automation commented Jul 25, 2024

dali546 commented Apr 19, 2024 •

edited

Loading

dali546 commented Apr 21, 2024 •

edited

Loading

aaythapa commented Apr 22, 2024 •

edited

Loading

aaythapa left a comment •

edited

Loading